🌱 [WIP] Use SPDY over websockets and fallback to direct SPDY on client side #11125
Conversation
k8s-ci-robot
added
the
area/provider/control-plane-kubeadm
|
/test help |
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@chrischdi: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/M
|
/test pull-cluster-api-e2e-conformance-main |
|
/retest flake |
|
|
||
| websocketDialer, err := portforward.NewSPDYOverWebsocketDialer(req.URL(), d.restConfig) | ||
| if err != nil { | ||
| return nil, err |
| @@ -26,7 +26,9 @@ import ( | |||
| "github.com/pkg/errors" | |||
There was a problem hiding this comment.
nit: Could it be that PR title and description are slightly incorrect?
It seems to me that we are always using SPDY. The question is just if it's over webhooks or directly?
| dialer := spdy.NewDialer(d.upgrader, &http.Client{Transport: d.proxyTransport}, "POST", req.URL()) | ||
| spdyDialer := spdy.NewDialer(d.upgrader, &http.Client{Transport: d.proxyTransport}, "POST", req.URL()) | ||
|
|
||
| websocketDialer, err := portforward.NewSPDYOverWebsocketDialer(req.URL(), d.restConfig) |
There was a problem hiding this comment.
Do we know if this only works with certain Kubernetes apiserver versions?
There was a problem hiding this comment.
Without diving to deep into the upstream k/k PR it looks to me like there is a server-side feature-gate to make this possible (PortForwardWebsockets) and it's only enabled per default with 1.31?
Is this correct?
xref:
- PortForward over Websockets Graduates to Beta kubernetes/kubernetes#125528
- https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/
If yes, this means for Kubernetes < 1.31 we are now with every single reconcile trying the websocketDialer but it won't work. Sounds like a lot of unnecessary "work" (not sure what it's exactly doing, requests?). We should probably try to avoid this
There was a problem hiding this comment.
+1
Wondering if we should have two dials method and invoke them depending on the k8s version, or we should have a cache (might be a TTL cache) where we track IP that do not support web sockets
There was a problem hiding this comment.
We maybe should configure the fallback depending on the k8s version.
< 1.31 : spdy first, websocket second
>= 1.31 : websocket first, spdy second
?
There was a problem hiding this comment.
That would be the simple option, it just doesn't take care of the case where someone disables the feature gate in 1.31.
Do we have an idea of how high the impact is of falling back on every single reconcile?
There was a problem hiding this comment.
Ah wait. We're talking about KCP, so KCP would actually know if someone enables/disables the feature gate (and not just the default is used). Although this could be tricky during rollouts, but probably fine if we just temporarily fallback a few additional times (should be okay as long as we don't permanently try to do something that has no chance of working?)
| } | ||
|
|
||
| // First attempt tunneling (websocket) dialer, then fallback to spdy dialer. | ||
| dialer := portforward.NewFallbackDialer(websocketDialer, spdyDialer, func(err error) bool { |
There was a problem hiding this comment.
I assume we can't tell from green e2e tests that websocketDialer worked, right? (it could be that we are always falling back to the spdyDialer, which I assume would lead to a lot of unnecessary stuff being done)
Would it make sense to check it once with Tilt manually? (if not already done)
|
Moving back to WIP |
k8s-ci-robot
changed the title
|
Yeah we noticed through 1.31 relase notes that the server-side feature is now enabled per default. |
chrischdi
changed the title
What this PR does / why we need it:
In Kubernetes they switched over to using websockets per default instead of SPDY and only fallback to spdy:
/area provider/control-plane-kubeadm
Note: The inmemory provider currently only supports SPDY on server-side.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #